[skrifa] FontRef instead of TableProvider #1079
Conversation
Per IM, changes skrifa to use `FontRef` directly rather than `TableProvider`. Extracts some shared functionality into a `BaseScaler` type for outlines in preparation for more autohinting work but no real functional changes.
There was a problem hiding this comment.
Adding Empty seems like it might have been a nice standalone, very small and easy to review
skrifa/src/outline/base.rs
Outdated
| impl<'a> BaseOutlines<'a> { | ||
| pub fn new(font: &FontRef<'a>) -> Option<Self> { | ||
| let hmtx = font.hmtx().ok()?; | ||
| let hvar = font.hvar().ok(); |
There was a problem hiding this comment.
perhaps worth a comment that we need hmtx, it's ok if there is no hvar
when the types don't popup it's very easy to miss that only one has a ?
skrifa/src/outline/base.rs
Outdated
| if let Some(hvar) = &self.hvar { | ||
| advance += hvar | ||
| .advance_width_delta(gid, coords) | ||
| // FreeType truncates metric deltas... |
There was a problem hiding this comment.
bonus points for reference
There was a problem hiding this comment.
This comment is actually stale so I removed it. We added rounding to the IVS itself in an earlier patch to match FT.
skrifa/src/outline/base.rs
Outdated
| lsb | ||
| } | ||
|
|
||
| pub fn cvt(&self) -> &[BigEndian<i16>] { |
There was a problem hiding this comment.
Both lsb and cvt merit a comment clarifying what that even is
skrifa/src/outline/base.rs
Outdated
|
|
||
| /// Common functionality for glyf, cff and autohinting scalers. | ||
| #[derive(Clone)] | ||
| pub(crate) struct BaseOutlines<'a> { |
There was a problem hiding this comment.
When I see cff or glyf structs holding base: BaseOutlines I'm left unclear what that does. I definitely wouldn't have anticipated the FontRef hides here.
Base is strongly associated with something you derive from for many of us and doesn't really conveying the common bits concept.
To ask the obvious, why not write a trait and implement it for FontRef? - cff and glyf holding font: FontRef and calling e.g. font.lsb would read more clearly to me. The main service this seems to serve is to avoid calling font.hmtx but if that's too slow ... maybe we could make it not be?
There was a problem hiding this comment.
Perf measurements suggest that looking up the tables on demand roughly doubles the cost of metrics retrieval. This may be acceptable for static text, but the animated variable case is one we like to highlight so wasting cycles for the sake of purity seems ill advised.
I renamed base -> common and BaseOutlines to OutlinesCommon for now. I agree this isn't ideal but I'd like to make progress on autohinting so I filed #1085 to revisit this in the future.
- rename base -> common and BaseOutlines -> OutlinesCommon - add some doc comments
Per IM, changes skrifa to use
FontRefdirectly rather thanTableProvider.Extracts some shared functionality into a
BaseOutlinestype for outlines in preparation for more autohinting work but no real functional changes.